Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patchinfo tests #3160

Merged
merged 3 commits into from May 30, 2017
Merged

Patchinfo tests #3160

merged 3 commits into from May 30, 2017

Conversation

bgeuken
Copy link
Member

@bgeuken bgeuken commented May 30, 2017

No description provided.

The generation of the package link was failing calling patchinfo_bread_crumb
in the helper spec. Thus I changed this in the helper.

====

ActionController::UrlGenerationError: No route matches {:action=>"show", :package=>"package_1", :project=>"project_1"}
@codecov
Copy link

codecov bot commented May 30, 2017

Codecov Report

Merging #3160 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3160      +/-   ##
==========================================
+ Coverage   88.93%   88.93%   +<.01%     
==========================================
  Files         263      263              
  Lines       17591    17587       -4     
==========================================
- Hits        15644    15641       -3     
+ Misses       1947     1946       -1
Flag Coverage Δ
#api 83.68% <33.33%> (+0.01%) ⬆️
#rspec 65.91% <100%> (+0.02%) ⬆️
#webui 64.47% <100%> (+0.57%) ⬆️

@@ -41,7 +41,10 @@
- if @issues.present?
%ul
- @issues.each do |issue|
= issue_link(issue)
- if issue[0] == "CVE"
= content_tag(:li, link_to((issue[1]).to_s, issue[2]) + ": #{issue[3]}")
Copy link
Contributor

@DavidKang DavidKang May 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why content_tag?, and not something like %li= link_to((issue[1]).to_s, issue[2]) + ": #{issue[3]}"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Didn't think of that

- if issue[0] == "CVE"
= content_tag(:li, link_to((issue[1]).to_s, issue[2]) + ": #{issue[3]}")
- else
= content_tag(:li, link_to("#{issue[0]}##{issue[1]}", issue[2]) + ": #{issue[3]}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@DavidKang DavidKang added the Frontend Things related to the OBS RoR app label May 30, 2017
@DavidKang
Copy link
Contributor

Nice work!

The code is only used once and not very complex. No need to have it in a
helper.
@bgeuken
Copy link
Member Author

bgeuken commented May 30, 2017

@DavidKang Changed it now

@bgeuken bgeuken merged commit 799c0c9 into openSUSE:master May 30, 2017
@bgeuken bgeuken deleted the patchinfo_tests branch May 30, 2017 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Things related to the OBS RoR app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants